-
Notifications
You must be signed in to change notification settings - Fork 12.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for suppressing project events #22590
Conversation
VS appears to have begun to receive these events as of #19864. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add test as well.
src/server/server.ts
Outdated
@@ -535,6 +535,7 @@ namespace ts.server { | |||
hrtime: process.hrtime, | |||
logger, | |||
canUseEvents, | |||
suppressProjectEvents, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels like this should be flags that indicates event to suppress given that this does not suppress ProjectInfoTelemetryEvent
which is project event
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the interest of keeping the name intelligible, I decided not to incorporate that. They also don't seem conceptually related to projects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then may be rename it to onlyTelemetryEvent or something as such?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not only telemetry. We also want TypingsInstaller events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could call it suppressDiagnosticEvents
, but then I'd feel obliged to let ProjectsUpdatedInBackgroundEvent
through (which is fine, it just requires a different implementation).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to suppressDiagnosticEvents
.
Note that only versions of VS patched to pass this additional flag will benefit from the change. |
Please add test to verify this functionality |
@sheetalkamat Better? |
Apparently, it is unwise to use GH's merge tool. I'll redo it locally. |
VS doesn't consume (e.g.) `syntaxDiag` events, so there's no reason to do the work to compute their payloads.
VS doesn't consume (e.g.)
syntaxDiag
events, so there's no reason to do the work to compute their payloads.